Apply the UARTE workaround for nrf91 and nrf53 #319
Conversation
It seems like endtx and txstopped race, and if we see txstopped first we return an error, causing writeln!(...).unwrap() to panic. This changes makes the UART work reliably on the nRF9160-PDK.
|
This also removes the check for txstopped, which doesn't work on the nrf9160 (the event is always set). |
| @@ -43,6 +43,16 @@ where | |||
| T: Instance, | |||
| { | |||
| pub fn new(uarte: T, mut pins: Pins, parity: Parity, baudrate: Baudrate) -> Self { | |||
| // Is the UART already on? It might be if you had a bootloader | |||
| if uarte.enable.read().bits() != 0 { | |||
| while uarte.events_txstopped.read().bits() == 0 { | |||
There was a problem hiding this comment.
events_txstopped only fires if whoever was using it before did a stoptx. This might spin forever if that's not the case.
There was a problem hiding this comment.
This is exactly why the errata workaround checks for rxenable/txenable, but we can't rely on those being present in nrf52 since they're undocuemnted...
There was a problem hiding this comment.
Ah, I see. With no check, I splatter the last line emitted by SPM. What's a good thing to check for here? Or do we not care that we configure the UART whilst it may still be enabled?
There was a problem hiding this comment.
there might be side effects if it's not disabled yeah :S
Maybe always do a stoptx? I think if tx is stopped stoptx still causes a new txstopped, so the loop is guaranteed to hang? I'm not 100% sure though
| if endtx || txstopped { | ||
| break; | ||
| } | ||
| while self.0.events_endtx.read().bits() == 0 { |
There was a problem hiding this comment.
the events_txstopped.reset(); above is no longer needed.
There was a problem hiding this comment.
Noted. Will remove.
| } | ||
|
|
||
| // Conservative compiler fence to prevent optimizations that do not | ||
| // take in to account actions by DMA. The fence has been placed here, | ||
| // after all possible DMA actions have completed. | ||
| compiler_fence(SeqCst); | ||
|
|
||
| if txstopped { |
There was a problem hiding this comment.
Maybe remove the Transmit error? it'd be a breaking change though.
There was a problem hiding this comment.
From the enum? Might as well leave it in for now in case we do need it again.
| // Lower power consumption by disabling the transmitter once we're | ||
| // finished. | ||
| self.0.tasks_stoptx.write(|w| | ||
| // `1` is a valid value to write to task registers. | ||
| unsafe { w.bits(1) }); | ||
|
|
||
| // Wait for transmitter to stop. |
There was a problem hiding this comment.
The tasks_stoptx write and this spin is unneeded. If you don't abort a tx, just receiving endtx already means the tx machinery is fully stopped.
There was a problem hiding this comment.
According to Nordic:
// Transmitter has to be stopped by triggering the STOPTX task to achieve
// the lowest possible level of the UARTE power consumption.
There was a problem hiding this comment.
Not that we seem to care about power consumption (as we busy-wait during a DMA transmit to the UART)
There was a problem hiding this comment.
Oh huh, strange. When I experimented with this, I saw UARTE using power as long as it's enabled, no matter what the tx/rx state is, so to have low power you have to disable it anyway... Embassy only waits for txstopped when disabling it, and only if the tx was aborted, which seems to work fine.
Seeing nordic has this code I'm now OK with leaving this in, though I don't fully get what the purpose is.
| @@ -109,7 +181,6 @@ where | |||
|
|
|||
| // Reset the events. | |||
| self.0.events_endtx.reset(); | |||
| self.0.events_txstopped.reset(); | |||
There was a problem hiding this comment.
Oops. Actually this was needed if we keep the blocking wait for txstopped below 😅 ... It should probably be moved next to that so it's more obvious why it's needed.
|
Build succeeded: |
See ttps://github.com/NordicSemiconductor/nrfx/blob/master/drivers/src/nrfx_uarte.c#L197